Skip to content

Fix issue 20065: support empty AliasSeq tuples in compiletime array literals#10197

Merged
dlang-bot merged 2 commits intodlang:stablefrom
FeepingCreature:fix/issue-20065-empty-aliasseq-in-array
Jul 22, 2019
Merged

Fix issue 20065: support empty AliasSeq tuples in compiletime array literals#10197
dlang-bot merged 2 commits intodlang:stablefrom
FeepingCreature:fix/issue-20065-empty-aliasseq-in-array

Conversation

@FeepingCreature
Copy link
Contributor

@FeepingCreature FeepingCreature commented Jul 19, 2019

Fixes:

import std.meta;

// Let me ask a question.
void main() {
    // This works.
    enum string[] array1 = [AliasSeq!("foo")]; 
    // So why doesn't this?
    enum string[] array2 = [AliasSeq!()];
}

@dlang-bot
Copy link
Contributor

dlang-bot commented Jul 19, 2019

Thanks for your pull request and interest in making D better, @FeepingCreature! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
20065 enhancement Empty AliasSeq can't be used to form compiletime array literal

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + dmd#10197"

@thewilsonator
Copy link
Contributor

This is a pretty simple fix, might as well target stable.

Copy link
Member

@WalterBright WalterBright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @Basile-z

@FeepingCreature FeepingCreature force-pushed the fix/issue-20065-empty-aliasseq-in-array branch from a7c259c to a864d6a Compare July 19, 2019 11:40
@FeepingCreature FeepingCreature changed the base branch from master to stable July 19, 2019 11:40
@FeepingCreature
Copy link
Contributor Author

Rebased on stable, std.meta import removed.

@thewilsonator thewilsonator dismissed WalterBright’s stale review July 19, 2019 11:42

phobos dependency removed.

@Geod24
Copy link
Member

Geod24 commented Jul 19, 2019

Looks like you also improved an error message in the process :)

@thewilsonator
Copy link
Contributor

Indeed

Test fail_compilation/fail8262.d failed: 
expected:
----
fail_compilation/fail8262.d(32): Error: cannot interpret `Tuple8262!1` at compile time
fail_compilation/fail8262.d(27): Error: template instance `fail8262.T8262!(Tuple8262!1)` error instantiating
fail_compilation/fail8262.d(19): Error: cannot implicitly convert expression `S(0)` of type `S` to `int`
----
actual:
----
fail_compilation/fail8262.d(32): Error: initializer must be an expression, not `Tuple8262!1`
fail_compilation/fail8262.d(27): Error: template instance `fail8262.T8262!(Tuple8262!1)` error instantiating
fail

@FeepingCreature FeepingCreature force-pushed the fix/issue-20065-empty-aliasseq-in-array branch from a864d6a to 21dcbbd Compare July 22, 2019 06:29
@thewilsonator
Copy link
Contributor

core.exception.AssertError@dmd/dinterpret.d(5074): Assertion failure

@FeepingCreature
Copy link
Contributor Author

Seems to be some places where CTFE is too defensive in what types of expressions it'll accept. Maybe fixed now? Let's run the tests.

@thewilsonator
Copy link
Contributor

Seems to like it, you still need to correct the error message above.

@FeepingCreature FeepingCreature force-pushed the fix/issue-20065-empty-aliasseq-in-array branch from f236a0c to ee70c63 Compare July 22, 2019 07:00
@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Jul 22, 2019

Now that this needed one or two more changes before going green, should this still go on stable or should I move back to master?

@thewilsonator
Copy link
Contributor

Stable is fine.

@dlang-bot dlang-bot merged commit c48b290 into dlang:stable Jul 22, 2019
@FeepingCreature FeepingCreature deleted the fix/issue-20065-empty-aliasseq-in-array branch July 22, 2019 08:17
@stefan-koch-sociomantic

dinterpret.d is not supposed to handle type-exps.
Nor' is the rest of the compiler really.
Prepare for unforeseen consequences.

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Jul 22, 2019

Ah damn, that sounds worrying. Noting that I don't really know the implications, and relied on the testsuite, afaics the problem happened originally because in enum string[] foo = [AliasSeq!()], the () is a type, not a value... the changes are just to allow the () to propagate to whatever part of the compiler notices this case ordinarily.

All the tests were green though, so...

Maybe I should have nailed things down so that only this specific case was allowed.

wilzbach pushed a commit to wilzbach/dmd that referenced this pull request Jul 30, 2019
…iterals (dlang#10197)

Fix issue 20065: support empty AliasSeq tuples in compiletime array literals
merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants